Skip to content

Use safeAsTime and safeAsDuration #1953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

k8scat
Copy link

@k8scat k8scat commented May 10, 2025

Completely fix #1846

@k8scat k8scat requested a review from a team as a code owner May 10, 2025 21:58
@CLAassistant
Copy link

CLAassistant commented May 10, 2025

CLA assistant check
All committers have signed the CLA.

@k8scat
Copy link
Author

k8scat commented May 12, 2025

@cretz Do you remind spending some time to have a look on this PR 😄

@@ -29,6 +30,15 @@ func safeAsTime(timestamp *timestamppb.Timestamp) time.Time {
}
}

// safeAsDuration ensures that a nil proto duration makes `AsDuration()` return 0.
func safeAsDuration(duration *durationpb.Duration) time.Duration {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this added? .AsDuration() should already be safe for nil

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this added? .AsDuration() should already be safe for nil

thanks for review, I will check this soon

Copy link
Author

@k8scat k8scat May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this added? .AsDuration() should already be safe for nil

.AsDuration is a func on *durationpb.Duration, and will panic if call it from nil varible, so it is like the .AsTime

image

image

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and will panic if call it from nil varible,

That is not true

	var d *durationpb.Duration
	fmt.Println(d.AsDuration())

prints 0s

so it is like the .AsTime

That is also not true

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and will panic if call it from nil varible,

That is not true

	var d *durationpb.Duration
	fmt.Println(d.AsDuration())

prints 0s

so it is like the .AsTime

That is also not true

It is my mistake! And I removed the safeAsDuration, please have a review again

@k8scat
Copy link
Author

k8scat commented May 18, 2025

@Quinn-With-Two-Ns any other question?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AsTime() on a nil pointer converts to Jan. 1st, 1970 at midnight UTC
3 participants